Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message for invalid input in Get()s #11081

Merged
merged 5 commits into from
Oct 30, 2020

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Oct 29, 2020

Before:

Exception: WithDeps(Inner(InnerEntry { params: {Specs, Console}, rule: Task(Task { product: List, can_modify_workunit: false, clause: [Addresses, ListSubsystem, Console], gets: [Get { output: UnexpandedTargets, input: Addresses }], func: pants.backend.project_info.list_targets:64:list_targets(), cacheable: false, display_info: DisplayInfo { name: "pants.backend.project_info.list_targets.list_targets", desc: Some("list goal"), level: Debug } }) })) did not declare a dependency on JustGet(Get { output: UnexpandedTargets, input: int })

After:

Engine traceback:
  in select
  in pants.backend.project_info.list_targets.list_targets
Traceback (most recent call last):
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/engine/internals/native.py", line 65, in generator_send
    res = func.send(arg)
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/backend/project_info/list_targets.py", line 79, in list_targets
    targets = await Get(UnexpandedTargets, Addresses, 123)
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/util/meta.py", line 182, in new_init
    prev_init(self, *args, **kwargs)
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/engine/internals/selectors.py", line 159, in __init__
    self.input = self._validate_input(input_arg1, shorthand_form=False)
  File "/Users/eric/DocsLocal/code/projects/pants/src/python/pants/engine/internals/selectors.py", line 201, in _validate_input
    f"Invalid Get. The third argument `{input_}` must have the exact same type as the "
TypeError: Invalid Get. The third argument `123` must have the exact same type as the second argument, pants.engine.addresses.Addresses, but had the type <class 'int'>.

Note that the stacktrace includes the offending Get, including the line and file where it's set. This is because we now eagerly validate in the Python constructor for Get.

We can only apply this new error message for non-union Get calls; the engine must handle type errors relating to unions because Python does not have knowledge of UnionMembership in the Get constructor.

Closes #8349.

Not sure why we had this in the first place

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@coveralls
Copy link

coveralls commented Oct 29, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling 7447d81 on Eric-Arellano:get-error into 47b583e on pantsbuild:master.

@rcuza
Copy link
Contributor

rcuza commented Oct 29, 2020

Feedback on the After Error Message

The "After" error message is clearer.

There is still some ambiguity about which input is at fault. Is UnexpectedTargets supposed to be a single object of type Addresses or the Addresses part? I guess it is "obvious" that it is the Addresses part, but just the fact that I had to think about this for a few cycles makes me think it could be clearer still.

Is it true that Get(A, B)expects A to be of type UnexpandedTargets and B to be of type Addresses? If yes, then maybe:

Exception: Invalid input to `Get(A, B)`. Expected `B` to be of type `Addresses`, but got `123` (type `int`) instead.

I am not sure what I am supposed to do with See the rule list_targets() from pants.backend.project_info.list_targets (line 64). I assume that I need to go to the pantsbuild/pants repo and look at the code. More helpful would be the line number of the offending Get(A, B) but maybe that information isn't available to the function throwing the exception.

@rcuza
Copy link
Contributor

rcuza commented Oct 29, 2020

re: pants.backend.project_info.list_targets (line 64): if we are going to send people to a goal to get more info, it at least needs a docstring.

I see that line 64 is dynamically determined which is très cool.

@Eric-Arellano
Copy link
Contributor Author

I am not sure what I am supposed to do with See the rule list_targets() from pants.backend.project_info.list_targets (line 64).

This is the @rule that contains the offending Get(). That specific one is only an example. It's because we have the line number of the @rule where the Get happens, but not the line of the Get() itself.

--

But, in a perfect world, I think the error message could look something like this:

Exception: Invalid input to Get(A, B) on line 64 of project.my_module.foo. Expected the Get's input to be of type B, but got 123 (type int).

I'll see what we can do.

@Eric-Arellano
Copy link
Contributor Author

I'll see what we can do.

Yeah I'm not finding a way in Python to automatically what line a constructor call happens at, that is where Get(A, B, b) is defined.

@rcuza
Copy link
Contributor

rcuza commented Oct 30, 2020

I am not sure what I am supposed to do with See the rule list_targets() from pants.backend.project_info.list_targets (line 64).

This is the @rule that contains the offending Get(). That specific one is only an example. It's because we have the line number of the @rule where the Get happens, but not the line of the Get() itself.

I thought I might be reading it out of context (i.e. that the module was in the code throwing the error), but after I hit comment.

--

But, in a perfect world, I think the error message could look something like this:

Get(A, B) on line 64 of project.my_module.foo. Expected the Get's input to be of type B, but got 123 (type int).

If you can't get the line and just the module you can say, "Exception: Invalid input to Get(A, B) in project.my_module.foo (line 64). Expected the Get's input to be of type B, but got 123 (type int)."

ty
)),
}
edges.entry_for(&dependency_key).cloned().ok_or_else(|| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has gotten pretty large, and would be good to extract into a helper function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to simplify the union code in a followup.

src/rust/engine/src/nodes.rs Outdated Show resolved Hide resolved
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano
Copy link
Contributor Author

PTAL. The error message was moved from Rust into the Python constructor. PR description is updated.

And rewrite part of it to use RuleRunner and to have better proximity.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Comment on lines +102 to +110
def test_invalid_get() -> None:
# Bad output type.
assert_invalid_get(
lambda: Get(1, str, "bob"), # type: ignore[call-overload, no-any-return]
expected=(
"Invalid Get. The first argument (the output type) must be a type, but given "
f"`1` with type {int}."
),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While these tests are brittle, I argue that's a feature. We know we need better error messages for the engine - this brittleness ensures we don't accidentally revert on the progress made in this PR.

Of course, we can rewrite the test if we want to reword the message.

ty
)),
}
edges.entry_for(&dependency_key).cloned().ok_or_else(|| {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to simplify the union code in a followup.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano merged commit 6d3f301 into pantsbuild:master Oct 30, 2020
@Eric-Arellano Eric-Arellano deleted the get-error branch October 30, 2020 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type mismatches in await Gets give unclear error message
5 participants